Skip to content

SIL: let SingleValueInstruction only inherit from a single SILNode (2nd try) #35615

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 11 commits into from
Jan 27, 2021

Conversation

eeckstein
Copy link
Contributor

After fixing a bug, this reinstates #35554 (after reverted by #35600).

Letting SingleValueInstruction only inherit from a single SILNode removes the ambiguity when casting from a SingleValueInstruction to SILNode, which makes the code simpler. E.g. the "isRepresentativeSILNode" logic is not needed anymore.
Also, it reduces the size of the most used instruction class - SingleValueInstruction - by one word.

Conceptually, SILInstruction is still a SILNode. But implementation-wise SILNode is not a base class of SILInstruction anymore.
Only the two sub-classes of SILInstruction - SingleValueInstruction and NonSingleValueInstruction - inherit from SILNode. SingleValueInstruction's SILNode is embedded into a ValueBase and its relative offset in the class is the same as in NonSingleValueInstruction (see SILNodeOffsetChecker).
This makes it possible to cast from a SILInstruction to a SILNode without knowing which SILInstruction sub-class it is.
Casting to SILNode cannot be done implicitly, but only with an LLVM cast or with SILInstruction::asSILNode(). But this is a rare case anyway.

Only the last commit of this PR does the actual SILNode change. All previous commits contain small cleanups, which are required for the change, but are good for their own.

Replace the `isa(SILNode *)` with `isa(SILInstruction *)` and `isa(SILValue)`.
This is much clearer and it also works if the SILValue is a MultiValueInstructionResult of an apply instruction.

Also, use `isa` instead of `classof` in canOptimize()
Split `markValueLive(SILNode *)` into `markValueLive(SILValue)` and `markInstructionLive(SILInstruction*)`
… when checking for invariant arguments.

Use `getDefiningInstruction` instead of casting a a SILValue to SingleValueInstruction.
…om SILNode* to SILValue

A small cleanup, NFC.
This resolves a FIXME.
Also, use `getDefiningInstruction()` instead of `getRepresentativeSILNodeInObject()`

NFC
This removes the ambiguity when casting from a SingleValueInstruction to SILNode, which makes the code simpler. E.g. the "isRepresentativeSILNode" logic is not needed anymore.
Also, it reduces the size of the most used instruction class - SingleValueInstruction - by one pointer.

Conceptually, SILInstruction is still a SILNode. But implementation-wise SILNode is not a base class of SILInstruction anymore.
Only the two sub-classes of SILInstruction - SingleValueInstruction and NonSingleValueInstruction - inherit from SILNode. SingleValueInstruction's SILNode is embedded into a ValueBase and its relative offset in the class is the same as in NonSingleValueInstruction (see SILNodeOffsetChecker).
This makes it possible to cast from a SILInstruction to a SILNode without knowing which SILInstruction sub-class it is.
Casting to SILNode cannot be done implicitly, but only with an LLVM `cast` or with SILInstruction::asSILNode(). But this is a rare case anyway.
@eeckstein
Copy link
Contributor Author

@swift-ci test

@eeckstein eeckstein merged commit c8766a0 into swiftlang:main Jan 27, 2021
@eeckstein eeckstein deleted the simplify-silnode branch January 27, 2021 19:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant